---
title: Tech debt
summary: CRITICAL and HIGH severity findings from analysis.
status: draft
owner: Not determined from source
last_reviewed: 2026-09-22
---

# Tech debt

`ADUserServices` carries one CRITICAL finding — a hard-coded default Active Directory account password — and a wide HIGH-severity band spanning contract stability, unauthenticated directory disclosure, spoofable authorization input, silent-failure logic across several mutating operations, and a pipeline artifact-naming collision. A supplementary MEDIUM table records additional findings for completeness; CRITICAL and HIGH below are the mandatory register.

## CRITICAL

| Finding | Component | Impact | Recommended action |
| --- | --- | --- | --- |
| SEC-05 — Hard-coded default Active Directory account password literal, substituted when the caller supplies an empty `inPassword`; the account is then created `.Enabled = True` | `clsADUserServices.AddUser`, the `If inPassword = String.Empty Then` branch (`src\ADUserServices\clsADUserServices.asmx.vb`, within lines 118–122) | Every account created without an explicit password shares one publicly-committed, immediately-usable credential | Move the default to the secure configuration store (or require an explicit password and reject empty input); rotate affected accounts; the literal value is not reproduced in this documentation |

## HIGH

| Finding | Component | Impact | Recommended action |
| --- | --- | --- | --- |
| TD-01 — Published contract bound to the framework default namespace `http://tempuri.org/` | `clsADUserServices.asmx.vb` line 38 (`<System.Web.Services.WebService(Namespace:="http://tempuri.org/")>`) | The WSDL target namespace and every message/SOAP-action namespace collide with any other service that never changed the default; changing it later breaks every generated consumer proxy already in the field | Assign a stable, organization-specific namespace before any further consumer onboarding; treat the change as a breaking-contract migration for existing consumers |
| TD-18 — `AddUser` continues after a caught `userPrin.Save()` failure, operating on an unsaved principal | `clsADUserServices.AddUser`, lines 145–152 | A save failure sets `Response = Failure` but the method proceeds into the directory-attribute block against a principal that was never persisted, risking inconsistent partial state | Return immediately after a `Save()` failure instead of falling through to the attribute-write block |
| TD-19 — `ChangeUserAccount` changes to `company`/`info` never set the `modified` flag, so `CommitChanges()` is skipped silently while the operation returns `Success` | `clsADUserServices.ChangeUserAccount`, lines 435–457 | A caller believes an attribute change succeeded when nothing was committed to the directory | Set `modified = True` for every directory-level attribute assignment, not only the principal-level ones |
| TD-20 — `DeleteUserFromGroup`'s `Catch` block is a deliberate swallow (`'gobble`), so any directory failure returns `Success` | `clsADUserServices.DeleteUserFromGroup`, lines 331–333 | A genuine directory failure is indistinguishable from a completed group removal | Distinguish "user not a member" from a genuine exception; return a failure sentinel for the latter |
| TD-21 — `GetUserGroups`/`GetUserResources` empty `Catch` blocks return an empty `ArrayList` on any failure | `clsADUserServices.asmx.vb` lines 574–576, 610–612 | A directory outage is indistinguishable from "user has no groups/resources" to every caller | Surface a distinct failure signal instead of swallowing the exception into an empty result |
| TD-27 — `ArtifactNameExternal` is set to `app-internal`, the same value as `ArtifactNameInternal`, so both build variants publish to one artifact name | `pipelines\build-vars.yml` | The two deployment variants cannot be released as distinguishable artifacts; a consumer of build history cannot tell which variant an artifact came from | Assign a distinct artifact name to the External variant |
| SEC-01 — IIS anonymous authentication enabled for both deployment variants while `Web.config` declares `Windows` authentication and defines no `<authorization>` element | `pipelines\release.yml` (`IISAnonymousAuthInternal: true`, `IISAnonymousAuthExternal: true`); `src\ADUserServices\Web.config` | The declared Windows-authentication mode is not enforced; the only effective barrier on the mutating surface is the IP allow-list | Add an explicit `<authorization>` policy, or confirm and document that anonymous access combined with the allow-list is the intended posture |
| SEC-02 — Eleven directory-reading operations carry no application-layer authorization, and nine of them disclose real Active Directory data | `clsADUserServices.asmx.vb` — `DoesUserExist`, `DoesUserHaveResource`, `GetUserEmail`, `GetUserGroups`, `GetUserResources`, `GetUserFAX`, `GetUserLastLogon`, `GetUserPrinter`, `IsUserInGroup` (plus `HelloWorld`, `CheckAccess` with no data disclosure) | Any caller that can reach the endpoint can read AD attributes and group/resource membership across both the internal and external domains | Apply the same `CheckAccess` gate, or an equivalent authorization construct, to the disclosing read operations |
| SEC-03 — The allow-list trusts `HTTP_X_FORWARDED_FOR` in preference to `REMOTE_ADDR`, with no trusted-proxy validation and no multi-hop parsing | `clsADUserServices.GetIncomingIp`, lines 798–806, consumed by `CheckAccess` lines 752–776 | A caller able to set that header can present an arbitrary source address and pass the allow-list check | Validate the header only from a known reverse proxy, or rely on `REMOTE_ADDR` |
| SEC-06 — Plaintext passwords are accepted as SOAP string parameters (`AddUser.inPassword`, `ChangeUserPassword`/`ChangeUserPasswordOrGetException`'s `inNewPassword`), with no transport security evidenced in the repository | `clsADUserServices.asmx.vb` — `AddUser`, `ChangeUserPassword`, `ChangeUserPasswordOrGetException`, private `ChangePassword` | A caller-supplied password travels in the request body with protection dependent entirely on a transport binding this repository does not declare | Require TLS at the endpoint and confirm the deployed binding enforces it |
| SEC-08 — A full directory-administration surface (create, modify, delete, unlock, re-password, re-group) sits behind a single IP allow-list with no user identity, no entitlement model, and no audit trail | `clsADUserServices.asmx.vb`, all eight mutating operations | Every privileged action is attributed to one shared app-pool identity with no way to trace which caller performed it | Introduce caller identity and audit logging in addition to the network-origin gate |
| SEC-16 — `AddUserToGroup`/`DeleteUserFromGroup` allow unconstrained group-membership changes, with no allow-list, name-pattern restriction, or exclusion of privileged/`*Resource*` groups | `clsADUserServices.AddUserToGroup`, `DeleteUserFromGroup` | A complete account-creation-to-entitlement escalation chain exists within one endpoint, gated only by network origin — an account created with the default password (SEC-05, CRITICAL) can be added to any resolvable group | Constrain which groups may be targeted by these operations, particularly privileged and `*Resource*` groups |
| SEC-17 — `DoesUserHaveResource` (transitive, via `GetAuthorizationGroups`) and `IsUserInGroup` (direct membership only, via `Members.Contains`) answer the same kind of authorization question inconsistently | `clsADUserServices.DoesUserHaveResource`, `IsUserInGroup`, `GetGroupPrincipal` | A consumer relying on one oracle over the other receives a different authorization answer for the same user/group, and both oracles are ungated | Reconcile the two operations on a single membership semantic, or document the difference prominently on the contract |
| SEC-19 — Password operations (`ChangeUserPassword`, `ChangeUserPasswordOrGetException`) are resets, not changes: no current-password verification, and no complexity/history validation in application code | `clsADUserServices.ChangePassword` | Any allow-listed origin can set the password of any external-domain account without proving knowledge of the existing one | Require current-password verification or an equivalent step-up check before resetting |
| SEC-12 — The 13-track `allowedips` allow-list is committed in cleartext (`environmentsettings\settings.yml`) alongside the spoofable `HTTP_X_FORWARDED_FOR` check that consumes it | `environmentsettings\settings.yml`, consumed by `clsADUserServices.GetAllowedIPs` | In combination with SEC-03, the repository publishes the exact `HTTP_X_FORWARDED_FOR` values an attacker needs to pass the allow-list for every environment; the storage mechanism itself is appropriate for a non-secret setting, so the finding is the composition, not the storage | Address the composite risk primarily by fixing SEC-03 (the spoofable header preference); consider whether publishing the full per-environment list is necessary |
| SEC-14 — No requester attribution on any directory mutation; every account creation, deletion, attribute change, password reset, unlock, and group-membership change executes as one shared app-pool identity | `clsADUserServices` — all directory helpers and all mutating operations | Directory-side audit records cannot distinguish which caller, environment, or origin performed a given mutation, and this is compounded by the same service account being used for non-prod and prod (SEC-07) | Introduce caller identity propagation and correlate it with directory-side audit logs |
| SEC-15 — Nine ungated operations (`GetUserEmail`, `GetUserFAX`, `GetUserPrinter`, `GetUserLastLogon`, `GetUserGroups`, `GetUserResources`, `IsUserInGroup`, `DoesUserExist`, `DoesUserHaveResource`) accept a caller-supplied `inUserId` and domain flag with no authentication, exposing both the internal (`corp.saif.com`) and external forests to unauthenticated enumeration | `clsADUserServices` — the nine ungated directory operations | Any unauthenticated caller can enumerate user existence, contact/attribute data, and — via `GetUserGroups` — the complete transitive authorization-group list for any named user across both forests | Apply the same `CheckAccess` gate, or an equivalent authorization construct, to these operations |

## MEDIUM

| Finding | Component | Impact | Recommended action |
| --- | --- | --- | --- |
| SEC-04 — `CheckAccess` is a published, unauthenticated operation that discloses the caller's observed remote address and the allow-list decision | `clsADUserServices.CheckAccess`, lines 752–776 | Provides an oracle for probing which addresses are on the allow-list | Gate or remove `CheckAccess` from the published contract |
| SEC-07 — App-pool service-account credentials are correctly externalized to a variable group, but the same account is used for non-prod and prod | `pipelines\release.yml` | A non-production compromise carries the same directory privileges as production | Separate the prod and non-prod service-account identities |
| SEC-09 — No audit logging of any mutating operation exists, despite the `log4net 2.0.5` package reference | `clsADUserServices.asmx.vb` (entire file); `packages.config` | No record of who performed any account creation, deletion, password reset, or group change | Initialize and call a logging framework for every mutating operation |
| SEC-10 — Allow-list comparison is raw, case-sensitive, untrimmed string equality | `clsADUserServices.CheckAccess`, `GetAllowedIPs` | A multi-hop `HTTP_X_FORWARDED_FOR` value or a whitespace-padded allow-list entry silently fails to match | Parse and normalize both sides before comparison |
| SEC-11 — Local-request bypass (`HttpContext.Current.Request.IsLocal`) grants full mutating access ahead of the allow-list | `clsADUserServices.CheckAccess` | Any other process or site co-hosted on the same server obtains the full directory-administration surface | Reconsider whether the local-request bypass is required, or scope it more narrowly |
| TD-12 — No null/empty guard before `Split(",")` on the allow-list setting | `clsADUserServices.GetAllowedIPs`, lines 791–796 | A missing or unreachable setting propagates as an unhandled exception (framework SOAP fault) instead of the `AccessDenied` sentinel, across all eight gated operations | Add a null/empty guard and fail closed with the `AccessDenied` sentinel |
| TD-13 — `debug="true"` in the checked-in `Web.config`, removed only by the `Release` transform | `src\ADUserServices\Web.config` | The debug flag is live for any deployment that does not run the `Release` transform, even though both pipeline variants currently do | Remove `debug="true"` from the base file so the safe state does not depend on the transform running |
| TD-04 — `FindUserInResource` is dead code; recursion passes a `Principal` into a `ByRef GroupPrincipal` parameter (legal only under `Option Strict Off`); `Throw ex` resets the stack trace | `clsADUserServices.asmx.vb` lines 898–925 | Misleading dead code that would misbehave if ever reactivated | Remove the dead method |
| TD-05 — Untyped `ArrayList` returns on `GetUserGroups`/`GetUserResources` | `clsADUserServices.asmx.vb` lines 557, 583 | The response has no schema a consumer can rely on; the generated proxy exposes `object[]` | Change the return type to a typed `String()` or a typed collection |
| TD-07 — `m_LastMessage` captures directory-failure detail that no operation ever returns or logs | `clsADUserServices.asmx.vb` lines 56, 836, 850, 855, 888 | Diagnostic detail for every directory failure is captured and then discarded | Surface the captured detail through logging (see SEC-09) |
| TD-08 — Eight referenced NuGet packages exercise zero symbols in source (`Autofac`, `FluentValidation`, `log4net`, `Newtonsoft.Json`, `RestSharp`, `SAIF.HashUtil`, `SAIF.Web.API.Clients`, `SAIF.Framework.Common`) | `packages.config`, `ADUserServices.vbproj` | Unused dependency surface increases restore weight and transitive-vulnerability exposure with no corresponding benefit | Remove unused package references |
| TD-14 — `Option Strict Off`/`strict="false"` permits late binding throughout (`de.Invoke("SetPassword", oPassword)`, `deGroup.Invoke("Remove", New Object() {deUser.Path})`, `de.Properties.Item("mail").Value` assigned to `String`, `.HighPart`/`.lowpart`) | `ADUserServices.vbproj`, `Web.config`, `clsADUserServices.asmx.vb` lines 330, 516, 546, 668–669 | Signature and type errors in these call sites surface only at run time, not at build | Enable `Option Strict On` incrementally and resolve the resulting diagnostics |
| TD-22 — `DoesUserExist` cannot distinguish "user absent" from "directory unreachable" | `clsADUserServices.asmx.vb` lines 342–353, 843–866 | A directory outage is reported identically to a genuinely absent user | Surface the captured `m_LastMessage` detail or a distinct failure signal |
| SEC-13 — Authorization fails open on the read surface and faults on the write surface when the environment service is unreachable | `clsADUserServices.GetAllowedIPs`, `CheckAccess` | An environment-service outage disables administration while leaving all eleven ungated disclosure operations fully available | Add a guard so the failure mode is consistent and intentional (see TD-12) |
| SEC-18 — `GetUserResources`'s production/non-production scope is chosen by the caller's `inProdEnvironment` flag rather than the deployment environment | `clsADUserServices.GetUserResources` | A misconfigured or hostile consumer can request the production entitlement list from a non-production call path | Derive the scope from the deployment environment instead of a caller-supplied flag |
| SEC-20 — No repository-declared transport or host protection (`requireSSL`, `<httpRuntime>` hardening, active `<customErrors>`) | `src\ADUserServices\Web.config`, `Web.Debug.config`, `Web.Release.config` | Whether TLS or other host-level hardening fronts this directory-administration endpoint is not determinable from source | Confirm and, if absent, add explicit transport/host hardening at the deploy-template or IIS level |

> **Note:** Statements marked with * were inferred from code structure, naming conventions, or
> contextual evidence where direct confirmation was not available in source artifacts. Statements
> marked with ** could not be confirmed; treat as unknown until verified.

## Related documentation
- [Service index](index.md)
- [Overview](overview.md)
- [Architecture](architecture.md)
- [Activation and hosting](activation-hosting.md)
- [Operations](operations.md)
- [Operations and processes](operations-processes.md)
- [Authentication and authorization](authentication-authorization.md)
- [Business rules](business-rules.md)
- [Configuration](configuration.md)
- [Dependencies](dependencies.md)
- [Faults and error pipeline](faults.md)
- [Operational runbook](operational-runbook.md)

## Sources
- `src\ADUserServices\clsADUserServices.asmx.vb`
- `src\ADUserServices\Web.config`
- `src\ADUserServices\packages.config`
- `pipelines\build-vars.yml`
- `pipelines\release.yml`
- `environmentsettings\settings.yml`
- `.saifdocgen\state\runs\20260922-101326-aduserservices\analysis\aduserservices-analysis.md`
- `.saifdocgen\state\runs\20260922-101326-aduserservices\analysis\aduserservices-security-analysis.md`
- `.saifdocgen\state\runs\20260922-101326-aduserservices\analysis\aduserservices-config-analysis.md`